Skip to content

gh-99240: Fix double-free bug in Argument Clinic str_converter generated code - #99241

Merged
miss-islington merged 13 commits into
python:mainfrom
colorfulappl:fix_ac_str_converter_cleanup
Nov 24, 2022
Merged

gh-99240: Fix double-free bug in Argument Clinic str_converter generated code#99241
miss-islington merged 13 commits into
python:mainfrom
colorfulappl:fix_ac_str_converter_cleanup

Conversation

@colorfulappl

@colorfulappl colorfulappl commented Nov 8, 2022 •
edited by miss-islington
Loading

Copy link
Copy Markdown
Contributor

Fix double-free bug mentioned at #99240,
by moving memory clean up out of "exit" label.

Automerge-Triggered-By: GH:erlend-aasland

@bedevere-bot bedevere-bot mentioned this pull request Nov 8, 2022
Closed
@bedevere-bot

ghost commented Nov 8, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

gpshead
gpshead approved these changes Nov 9, 2022
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall this looks good, only a minor Python code modernization suggestion.

I do think your second bullet from the bug "If function _PyArg_ParseStack parses failed, assign all the parsed arguments to "NULL" after they are freed, this should be done in _PyArg_ParseStack." is also worth doing to make bugs like this less likely to occur. That can be done in its own PR.

Comment thread Tools/clinic/clinic.py Outdated
@gpshead gpshead self-assigned this Nov 9, 2022
@gpshead gpshead added topic-argument-clinic type-bug An unexpected behavior, bug, or error labels Nov 9, 2022
@gpshead

ghost commented Nov 9, 2022

Copy link
Copy Markdown
Member

Of particular note, since the generated files check in CI passed, does that mean we don't have any actual standard library argument clinic uses of this str conversion via codec functionality yet?

@colorfulappl

ghost commented Nov 9, 2022 •
edited by colorfulappl
Loading

Copy link
Copy Markdown
Contributor Author

does that mean we don't have any actual standard library argument clinic uses of this str conversion via codec functionality yet?

Yes, I have searched this in CPython source code and didn't find any usage of argument clinic str conversion with "encoding" parameter setting.

And test for this functionality is added in #96178, which has not been merged yet.

erlend-aasland
erlend-aasland reviewed Nov 9, 2022
View reviewed changes
Comment thread Tools/clinic/clinic.py Outdated
@erlend-aasland

ghost commented Nov 9, 2022 •
edited by erlend-aasland
Loading

Copy link
Copy Markdown
Contributor

Also, my preference, as stated in #96178 (comment), would be to first merge that PR (with a minimal test suite), and then for each bugfix PR (such as this one) add both the bugfix and the complementing test. So, I'm suggesting putting this on hold until #96178 is merged.

@colorfulappl

ghost commented Nov 24, 2022

Copy link
Copy Markdown
Contributor Author

Added Argument Clinic functional test (#96002).

arhadthedev
arhadthedev reviewed Nov 24, 2022
View reviewed changes
Comment thread Modules/clinic/_testclinic.c.h Outdated
erlend-aasland
erlend-aasland reviewed Nov 24, 2022
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor nitpicks :)

Comment thread Misc/NEWS.d/next/Library/2022-11-08-15-54-43.gh-issue-99240.MhYwcz.rst Outdated
Comment thread Modules/_testclinic.c Outdated
Comment thread Tools/clinic/clinic.py Outdated
colorfulappl and others added 3 commits November 24, 2022 19:21
colorfulappl Erlend E. Aasland
Simplify news
946ad3f 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
colorfulappl Erlend E. Aasland
Delete unnecessary function doc
7767139 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
colorfulappl Erlend E. Aasland
Simplify function doc
9d08af5 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@colorfulappl
colorfulappl force-pushed the fix_ac_str_converter_cleanup branch from 45886f1 to 9d08af5 Compare November 24, 2022 11:30
erlend-aasland
erlend-aasland reviewed Nov 24, 2022
View reviewed changes
Comment thread Tools/clinic/clinic.py Outdated
colorfulappl added 2 commits November 24, 2022 21:32
colorfulappl
Polish comment
485c7aa
colorfulappl
Merge branch 'main' into fix_ac_str_converter_cleanup
18ae693 
# Conflicts:
#	Lib/test/test_clinic.py
#	Modules/_testclinic.c
#	Modules/clinic/_testclinic.c.h
erlend-aasland
erlend-aasland approved these changes Nov 24, 2022
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@erlend-aasland

ghost commented Nov 24, 2022

Copy link
Copy Markdown
Contributor

I do think your second bullet from the bug "If function _PyArg_ParseStack parses failed, assign all the parsed arguments to "NULL" after they are freed, this should be done in _PyArg_ParseStack." is also worth doing to make bugs like this less likely to occur. That can be done in its own PR.

@colorfulappl, are you up for fixing _PyArg_ParseStack while you're at it?

@colorfulappl

ghost commented Nov 24, 2022

Copy link
Copy Markdown
Contributor Author

are you up for fixing _PyArg_ParseStack while you're at it?

I have not started yet, but I am willing to have a try. :)

@miss-islington

ghost commented Nov 24, 2022

Copy link
Copy Markdown
Contributor

Status check is done, and it's a success ✅.

@miss-islington
miss-islington merged commit 8dbe08e into python:main Nov 24, 2022
@bedevere-bot

ghost commented Dec 20, 2022

Copy link
Copy Markdown

GH-100352 is a backport of this pull request to the 3.11 branch.

@bedevere-bot

ghost commented Dec 20, 2022

Copy link
Copy Markdown

GH-100353 is a backport of this pull request to the 3.10 branch.

@jiridanek jiridanek mentioned this pull request Jan 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arhadthedev arhadthedev arhadthedev left review comments
@gpshead gpshead gpshead approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes

Assignees

@gpshead gpshead

Labels

topic-argument-clinic type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@colorfulappl @bedevere-bot @gpshead @erlend-aasland @miss-islington @arhadthedev